Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW] Use CMAKE_CUDA_ARCHITECTURES #7391

Merged

Conversation

robertmaynard
Copy link
Contributor

When using CMake >= 3.18 you now don't get CMP0104 warnings for each target that use CUDA.

To correct this issue I have cherry-picked a collection of changes from #7107 which updated cudf to use CMAKE_CUDA_ARCHITECTURES.

When using CMake < 3.18 cudf will transform CMAKE_CUDA_ARCHITECTURES into the correct flags and store it into CMAKE_CUDA_FLAGS.

When I was porting this over from !7107 I noticed that when cudf is built for multi arch ( sm60, sm70, sm80, ... ) it only compiles compute for the newest arch. This behavior is preserved, and !7107 will need to be updated with this change.

@robertmaynard robertmaynard requested a review from a team as a code owner February 16, 2021 19:10
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Feb 16, 2021
Copy link
Collaborator

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things generally look good. Should we consider to move to using the CUDA_ARCHITECTURES target property instead of manually passing flags to the CUDA compiler?

cpp/cmake/EvalGpuArchs.cmake Outdated Show resolved Hide resolved
@robertmaynard
Copy link
Contributor Author

Things generally look good. Should we consider to move to using the CUDA_ARCHITECTURES target property instead of manually passing flags to the CUDA compiler?

I think that move is reasonable. Maybe after / part of #7107?

It would allow better compilation flags control. If we ever needed targets built for a subset of archs we could.
It would be a bit more verbose, but less cmake 'magic' which is nice.

@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.19@0bc7e15). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #7391   +/-   ##
==============================================
  Coverage               ?   82.19%           
==============================================
  Files                  ?      100           
  Lines                  ?    16968           
  Branches               ?        0           
==============================================
  Hits                   ?    13947           
  Misses                 ?     3021           
  Partials               ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bc7e15...d319394. Read the comment docs.

@harrism harrism added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Feb 16, 2021
@harrism harrism changed the title cudf updated to use CMAKE_CUDA_ARCHITECTURES Use CMAKE_CUDA_ARCHITECTURES Feb 16, 2021
@harrism harrism added the 3 - Ready for Review Ready for review by team label Feb 16, 2021
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/CMakeLists.txt Show resolved Hide resolved
@robertmaynard
Copy link
Contributor Author

rerun tests

@robertmaynard robertmaynard changed the title Use CMAKE_CUDA_ARCHITECTURES [WIP] Use CMAKE_CUDA_ARCHITECTURES Feb 18, 2021
@robertmaynard robertmaynard changed the title [WIP] Use CMAKE_CUDA_ARCHITECTURES [REVIEW] Use CMAKE_CUDA_ARCHITECTURES Feb 19, 2021
@kkraus14 kkraus14 added breaking Breaking change and removed non-breaking Non-breaking change labels Feb 19, 2021
@kkraus14
Copy link
Collaborator

Pinged some folks offline regarding rapids-compose and devops scripts to make sure no one is using GPU_ARCHS before we merge this.

@kkraus14
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7a6e60e into rapidsai:branch-0.19 Feb 19, 2021
@robertmaynard robertmaynard deleted the correct_CMP0104_warnings branch February 19, 2021 19:22
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Feb 24, 2021
Based on #706 as they both modify `cmake/Modules/SetGPUArchs.cmake`

This brings rmm's handling of `CMAKE_CUDA_ARCHITECTURES` to match that is proposed for cudf in rapidsai/cudf#7391

Authors:
  - Robert Maynard (@robertmaynard)

Approvers:
  - Mark Harris (@harrism)
  - Keith Kraus (@kkraus14)

URL: #709
rapids-bot bot pushed a commit that referenced this pull request Feb 24, 2021
This eliminates the `Policy CMP0104 is not set` warning during the JNI build by using `CMAKE_CUDA_ARCHITECTURES` to specify the targeted CUDA architectures during the build.  This eliminates a lot of code replicated from the cpp build and reuses the `ConfigureCUDA` module added in #7391.  The architectures being used by the build are visible in the `mvn` output, e.g.:
```
     [exec] -- CUDF: Building CUDF for GPU architectures: 60-real;70-real;75
```

This also configures the CPU compiler to use the same flags as the cpp build which required fixing a number of warnings (e.g.: sign mismatch comparisons, unused variables, etc.) in the JNI code since warnings are errors in the build.

Authors:
  - Jason Lowe (@jlowe)

Approvers:
  - Robert (Bobby) Evans (@revans2)
  - Alessandro Bellina (@abellina)
  - MithunR (@mythrocks)

URL: #7425
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants